[MNG-8678] Concurrent executor ignores java.lang.Error subclasses - #13067
Conversation
Closes apache#10415 Widen catch blocks in ConcurrentLifecycleStarter from Exception to Throwable so that Error subclasses thrown during build steps are properly recorded in the build result instead of being silently swallowed.
| * @return {@code true} if the build must be halted | ||
| */ | ||
| private static boolean isFatal(List<Throwable> failures) { | ||
| return failures.stream().anyMatch(t -> t instanceof RuntimeException || !(t instanceof Exception)); |
There was a problem hiding this comment.
Nit / Documentation gap: The name isFatal and the predicate t instanceof RuntimeException || !(t instanceof Exception) are correct, but the Javadoc only explains what this method does, not why RuntimeException is treated as fatal alongside Error. A reader unfamiliar with the original design intent in handleBuildError will be confused: checked exceptions are "soft" failures, RuntimeExceptions and Errors are "hard" ones. Worth a single sentence in the @return tag:
| return failures.stream().anyMatch(t -> t instanceof RuntimeException || !(t instanceof Exception)); | |
| private static boolean isFatal(List<Throwable> failures) { | |
| // RuntimeExceptions are treated as system errors on par with Errors: | |
| // both indicate the JVM or framework is in an unexpected state and | |
| // further build steps are unlikely to succeed. | |
| return failures.stream().anyMatch(t -> t instanceof RuntimeException || !(t instanceof Exception)); | |
| } |
| * catch does not change it. | ||
| */ | ||
| @Test | ||
| void exceptionThrownByBuildStepIsRecordedAsBuildFailure() throws Exception { |
There was a problem hiding this comment.
Missing coverage: exceptionThrownByBuildStepIsRecordedAsBuildFailure uses IllegalStateException (a RuntimeException), so it tests the fatal path — the build will halt just like for an Error. There is no test that throws a checked exception and verifies that the reactor is not halted (i.e. the soft-failure path: isFatal returns false, event fires, blacklisting happens). That path existed before this PR and the widened catch (Throwable) should not affect it, but having it pinned would prevent a future regression in isFatal from silently breaking REACTOR_FAIL_AT_END for ordinary plugin failures.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
The follow-up commit cleanly addresses the previous review:
isFatalJavadoc now spells out the classification semantics (checked → soft, unchecked/Error → halt) — good, that was the main ask.- New
checkedExceptionThrownByBuildStepDoesNotHaltReactortest pins the soft-failure path withREACTOR_FAIL_AT_END, which is exactly the scenario that theisFatalextraction was designed to protect.
Backport is a faithful cherry-pick of the merged master PR #13055 plus these review improvements. No semantic divergence from the 4.0.x base. CI is still queued — assuming it goes green, this is ready to merge.
This review was generated by an AI agent, Hermès, on behalf of @gnodet.
2 participants
Backport of #13055 to maven-4.0.x.
Cherry-pick of c6d4df6 (master) — applied cleanly, no conflicts.